Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

OCM-10615 | Implement 'gcp wif-config update' command #667

Merged
merged 5 commits into from
Sep 27, 2024
Merged

OCM-10615 | Implement 'gcp wif-config update' command #667

merged 5 commits into from
Sep 27, 2024

Conversation

JakobGray
Copy link
Contributor

  1. Implement 'gcp wif-config update' command to re-apply GCP resources
  2. Filter version on 'create cluster' when using WIF
  3. Accept WIF displayName in WIF commands

Related issue: OCM-10615

renan-campos
renan-campos requested changes Sep 19, 2024
View reviewed changes
Copy link
Collaborator

@renan-campos renan-campos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requested changes pertain to code organization. The new function implementations and overall enhancements look good to me.

cmd/ocm/gcp/gcp-client-shim.go Outdated Show resolved Hide resolved
cmd/ocm/gcp/gcp-client-shim.go Outdated Show resolved Hide resolved
cmd/ocm/gcp/update-wif-config.go Outdated Show resolved Hide resolved
cmd/ocm/gcp/generate-wif-script.go Outdated Show resolved Hide resolved
cmd/ocm/gcp/update-wif-config.go Outdated Show resolved Hide resolved
@renan-campos
Copy link
Collaborator

Couple things I found while testing this out:

  1. The create.sh and apply.sh scripts have an error in them: looks like a space is missing between the project name and permissions.
  2. The logging of update needs some attention.
  • The sre role is getting updated every time it is called.
  • Granting of support is logged as occurring every time as well.
  • Workload identity federation pool and provider are logged as "already exists" but the other resources are not.

renan-campos
renan-campos previously requested changes Sep 25, 2024
View reviewed changes
Copy link
Collaborator

@renan-campos renan-campos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above comment.

@renan-campos
Copy link
Collaborator 

[rcampos@rcampos-thinkpadt14sgen2i ocm-cli]$ ./ocm gcp update wif-config rc-test
2024/09/25 13:27:45 Role "sre_managed_support" updated
2024/09/25 13:27:46 support access granted to [email protected]
2024/09/25 13:27:46 Workload identity pool 2e0vgthhfsdl1kbrbokvakum7r8tqm24 exists
2024/09/25 13:27:46 Workload identity provider oidc exists

@JakobGray
Copy link
Contributor Author

I've modified the logging so it won't log anything if no updates are done. I changed the createOrUpdateRoles so it adds missing permissions rather than replacing them. And I fixed the gcloud command and tested that it can run properly.

I tested with 3 changes:

  1. Deleted SA [email protected]
  2. Added a permission to role osd_deployer_v4.17
  3. Removed a permission on role openshift_gcp_pd_csi_driver_operator_v4.17

Running results in these logs:

go run ./cmd/ocm gcp update wif-config kms
2024/09/26 12:41:50 IAM service account osd-worker-g064 created
2024/09/26 12:41:54 Role "openshift_gcp_pd_csi_driver_operator_v4.17" updated

The result is

  1. [email protected] is recreated
  2. The added permission in osd_deployer_v4.17 is left
  3. The removed permission in openshift_gcp_pd_csi_driver_operator_v4.17 is added back

@ckandag ckandag changed the title Implement 'gcp wif-config update' command OCM-10615 | Implement 'gcp wif-config update' command Sep 27, 2024
@ckandag
Copy link
Collaborator

ckandag commented Sep 27, 2024

I scanned to make sure all of the PR review comments were addressed.
I locally verified there is no regression in the green paths i.e creation of a wif , creation of a basic gcp cluster
also verified version filtering for wif is working as expected.

I have not tested the scripts though. I am merging this MR but @JakobGray pls make sure to verify all the dry run scripts and post a comment here and address any issues post-merge

@ckandag ckandag self-requested a review September 27, 2024 03:17
@ckandag ckandag dismissed renan-campos’s stale review September 27, 2024 03:18

verified review comments are addressed

@ckandag ckandag merged commit e4aa770 into openshift-online:main Sep 27, 2024
4 of 5 checks passed
@JakobGray JakobGray deleted the enhancements branch September 30, 2024 19:26
ckandag added a commit that referenced this pull request Oct 15, 2024
@ckandag
Release v0.1.76
3622ae9 
-e034b6b Update Konflux references to 2418e94
-5066ea0 Filter wif configs in interactive mode (#660)
-878f5e3 Initial refactor to prepare to move the connection builder and config packages to ocm-common
-1ea2e05 lint
-2c66dc0 removes redundant api url
-65bf8cf Add role prefix flag on create wif-config (#662)
-a39ce2e Grant access to support group during WifConfig creation (#663)
-0275d67 Revert "Grant access to support group during WifConfig creation (#663)" (#664)
-7cddc94 Wif creation improvements, including logic to grant support access as part of wif creation. (#666)
-7f41626 Update Konflux references
-b9a750c UpdatesToKonflux (#668)
-e4aa770 OCM-10615 | Implement 'gcp wif-config update' command (#667)
-cf6e500 Dry-run wif config delete before tearing down cloud resources (#670)
-e18ea10 OCM-11842 | feat: Updates to support GCP-PSC clusters (#672)
-893acd5 wif-enable gcp-inquiries (#673)
-664b2c4 Replace wif dry-run flag with mode (#671)
-df87894 Update Konflux references (#669)
@ckandag ckandag mentioned this pull request Oct 15, 2024
Merged
renan-campos pushed a commit that referenced this pull request Oct 15, 2024
@ckandag
Release v0.1.76 (#674)
8b70707 
-e034b6b Update Konflux references to 2418e94
-5066ea0 Filter wif configs in interactive mode (#660)
-878f5e3 Initial refactor to prepare to move the connection builder and config packages to ocm-common
-1ea2e05 lint
-2c66dc0 removes redundant api url
-65bf8cf Add role prefix flag on create wif-config (#662)
-a39ce2e Grant access to support group during WifConfig creation (#663)
-0275d67 Revert "Grant access to support group during WifConfig creation (#663)" (#664)
-7cddc94 Wif creation improvements, including logic to grant support access as part of wif creation. (#666)
-7f41626 Update Konflux references
-b9a750c UpdatesToKonflux (#668)
-e4aa770 OCM-10615 | Implement 'gcp wif-config update' command (#667)
-cf6e500 Dry-run wif config delete before tearing down cloud resources (#670)
-e18ea10 OCM-11842 | feat: Updates to support GCP-PSC clusters (#672)
-893acd5 wif-enable gcp-inquiries (#673)
-664b2c4 Replace wif dry-run flag with mode (#671)
-df87894 Update Konflux references (#669)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ckandag ckandag ckandag approved these changes

@renan-campos renan-campos Awaiting requested review from renan-campos

Assignees

@JakobGray JakobGray

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@JakobGray @renan-campos @ckandag